Skip to content

Fix ApiCallDuration so that it measures the whole API call - #7338

Merged
alextwoods merged 6 commits into
masterfrom
alexwoo/fix_metrics
Sep 3, 2026
Merged

Fix ApiCallDuration so that it measures the whole API call#7338
alextwoods merged 6 commits into
masterfrom
alexwoo/fix_metrics

Conversation

@alextwoods

@alextwoods alextwoods commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Fix ApiCallDuration so it measures the whole API call

Motivation and Context

CoreMetric.API_CALL_DURATION understated latency, and did so differently on the sync and async clients.

The measurement lived in a request pipeline stage. The pipeline's input is the already-marshalled request, so the window
opened after marshalling had finished — contradicting the formula in the metric's own javadoc, which lists a
MARSHALLING_DURATION term.

The async client was worse. AsyncApiCallMetricCollectionStage was nested one builder deeper than its sync counterpart,
so the window also excluded the entire request-mutation chain: endpoint resolution, auth scheme resolution, request compression, checksums, header and query merging and user agent.

The two clients also disagreed on the afterExecution interceptors: inside the window on async, outside on sync.

Modifications

  • The measurement for ApiCallDuration is moved out of the pipeline entirely, up into the client handlers alongside API_CALL_SUCCESSFUL — which was already scoped to the whole call, so the two metrics had been reporting on different windows from the same collector. No pipeline arrangement can enclose marshalling, so this was the only available position.
  • afterExecution interceptors are now inside the window on both clients: an interceptor running as part of the call is part of the call.
  • SERVICE_ENDPOINT collection moved into the handlers' doExecute, the first point after the pipeline that still holds the ExecutionContext.
  • Both handlers skip the clock reads when the collector is a NoOpMetricCollector, the default when no publisher is configured, so the no-metrics path does not regress.

Testing

New ApiCallDurationWindowTest (test/codegen-generated-classes-test) injects a 300 ms delay into one phase at a time and asserts it appears in ApiCallDuration, for both clients.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.

License

  • I confirm that this pull request can be released under the Apache 2 license

One internal-API behaviour change: code driving AmazonSyncHttpClient or AmazonAsyncHttpClient directly, bypassing the
client handlers, no longer receives ApiCallDuration or SERVICE_ENDPOINT.

Testing

New ApiCallDurationWindowTest (test/codegen-generated-classes-test) injects a 300 ms delay into one phase at a time
and asserts it appears in ApiCallDuration, for both clients. This is deliberately stronger than asserting the
additivity formula over real timings: the phases at issue cost microseconds against a millisecond-scale call, so an
inequality passes whether or not they are counted — an additivity-only check would not have caught the sync defect.

Verified against the unpatched SDK: 5 of the 7 cases fail, and the 2 that pass are exactly the two that were already
correct
(sync endpoint resolution, async afterExecution).

Also added: ApiCallDurationAssertions, applied from CoreMetricsTest and BaseAsyncCoreMetricsTest, asserting
ApiCallDuration encloses each component metric on success, error and retry paths.

@alextwoods
alextwoods requested a review from a team as a code owner September 1, 2026 15:46
@alextwoods alextwoods added the no-api-surface-area-change Indicate there is no API surface area change and thus API surface area review is not required label Sep 1, 2026
@alextwoods alextwoods added the perf-improvement Label for PRs that contain performance improvement changes. label Sep 1, 2026
@Fred1155

Fred1155 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

With this change, the ApiCallDuration could be longer than configured apiCallTimeout since marshalling and endpoint resolution stuff does not count into the timeout. This could be confusing and worth mentioning that in somewhere, probably API_CALL_DURATION javadoc.

@alextwoods

Copy link
Copy Markdown
Contributor Author

With this change, the ApiCallDuration could be longer than configured apiCallTimeout since marshalling and endpoint resolution stuff does not count into the timeout. This could be confusing and worth mentioning that in somewhere, probably API_CALL_DURATION javadoc.

Great catch and callout! I've updated javadoc in both spots.

I'd love to fix that as well, as I think the current arrangement of it is misleading, but I think thats a larger breaking change because it could cause apiCallTimeout configurations that were previously allowing requests to succeed could cause requests to start failing (since it makes the timeout itself more strict).

@Fred1155

Fred1155 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

I'd love to fix that as well, as I think the current arrangement of it is misleading, but I think thats a larger breaking change because it could cause apiCallTimeout configurations that were previously allowing requests to succeed could cause requests to start failing (since it makes the timeout itself more strict).

Yeh I agree. I think a doc reminder is enough and we already called out that apiCallTimeout excludes marshaling, so although that is misleading, that is a documented behavior rather than a bug.

@alextwoods
alextwoods added this pull request to the merge queue Sep 2, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to Branch Protection failures Sep 2, 2026
You're not authorized to push to this branch. Visit "About protected branches" for more information.
@alextwoods
alextwoods enabled auto-merge September 2, 2026 19:16
@alextwoods
alextwoods added this pull request to the merge queue Sep 2, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Sep 2, 2026
@alextwoods
alextwoods added this pull request to the merge queue Sep 2, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Sep 2, 2026
@alextwoods
alextwoods enabled auto-merge September 2, 2026 23:56
@alextwoods
alextwoods added this pull request to the merge queue Sep 3, 2026
Merged via the queue into master with commit a42ea7a Sep 3, 2026
13 of 14 checks passed
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Sep 3, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

no-api-surface-area-change Indicate there is no API surface area change and thus API surface area review is not required perf-improvement Label for PRs that contain performance improvement changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants